Skip to content

Conversation

@buffalojoec
Copy link
Contributor

Problem

Working with the Firedancer conformance suite revealed another relatively inconsequential control flow mismatch between the BPF version of the Config program and its original builtin version.

Similar to #16, when the BPF version of the Config program attempts to deserialize ConfigKeys - this time from account state - there can be mismatches on error codes due to BPF program panics due to memory allocations.

In the case of a malformed account with a large ShortU16 vector length, but not enough data for the provided length, if the length causes too large of an allocation, the BPF program will panic (OOM), rather than returning ProgramError::InvalidAccountData like the builtin does.

Again, similar to #16, this would still cause an error, but this change ensures maximum backwards compatibility. Furthermore, writes to a config account's ConfigKeys are shielded by the input buffer restrictions.

Summary of Changes

Add a check for the provided ShortU16 vector length to the program's config account state deserialization, to catch any invalid buffers and avoid OOM panics in favor of backwards-compatible error codes.

@buffalojoec buffalojoec marked this pull request as ready for review November 4, 2024 11:09
@buffalojoec buffalojoec requested a review from joncinque November 4, 2024 11:09
@buffalojoec buffalojoec force-pushed the safe-deserialize-config-keys-state branch 2 times, most recently from e2b4b9f to ec19760 Compare November 4, 2024 12:36
@buffalojoec buffalojoec force-pushed the safe-deserialize-config-keys-state branch from ec19760 to db53895 Compare November 4, 2024 12:42
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense -- nice catch and fix!

@buffalojoec buffalojoec merged commit 8fdf608 into main Nov 4, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants